-
Notifications
You must be signed in to change notification settings - Fork 459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add env support to instrumentation kind #674
Add env support to instrumentation kind #674
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add/modify e2e test that verifies this feature? (It's simple to do :))
bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
Hi, Are there any guides to add e2e test? And what is the output of e2e test? |
I thinks i get i. will add e2e testcast later. |
@Duncan-tree-zhou Here are existing e2e tests, you can reference the python instrumentation test here. In Python inst e2e tests:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. It needs some cleanup, e2e tests and it can go in :)
For the e2e test just add a new variable to the instrumentation CR, app deployment and then verify the final result in the assert yaml. |
ba64543
to
afcf9e6
Compare
afcf9e6
to
dadd4da
Compare
thank you @pavolloffay @VineethReddy02 for helping, the testcases finally passed. --- PASS: kuttl (50.00s)
--- PASS: kuttl/harness (0.00s)
--- PASS: kuttl/harness/daemonset-features (6.40s)
--- PASS: kuttl/harness/smoke-simplest (9.24s)
--- PASS: kuttl/harness/smoke-statefulset (24.29s)
--- PASS: kuttl/harness/smoke-pod-annotations (17.89s)
--- PASS: kuttl/harness/instrumentation-python (25.81s)
--- PASS: kuttl/harness/statefulset-features (25.82s)
--- PASS: kuttl/harness/smoke-sidecar (28.83s)
--- PASS: kuttl/harness/instrumentation-nodejs (22.63s)
--- PASS: kuttl/harness/instrumentation-java (12.74s)
--- PASS: kuttl/harness/targetallocator-features (38.43s)
--- PASS: kuttl/harness/smoke-targetallocator (39.45s)
|
4df7388
to
03b20b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, one outstanding item is to clarify precedence order and how the env vars are overridden.
ca581b0
to
4f09121
Compare
4f09121
to
29cbdb1
Compare
Hi @pavolloffay , I thinks all problems had been resolved. Description in doc for env vars is like the following paragraphs, please have a review. Env defines common env vars. There are four layers for env vars' definitions and the precedence order is: `original container env vars` > `language specific env vars` > `common env vars` > `instrument spec configs' vars`. If the former var had been defined, then the other vars would be ignored. Env defines java/python/nodejs specific env vars. There are four layers for env vars' definitions and the precedence order is: `original container env vars` > `language specific env vars` > `common env vars` > `instrument spec configs' vars`. If the former var had been defined, then the other vars would be ignored. |
@Duncan-tree-zhou thanks, the PR looks good. The CI failed on manifest validation. Please re-generate the bundle. |
@pavolloffay updated |
@Duncan-tree-zhou CI failed |
bf38693
to
8ef1690
Compare
Hi @pavolloffay, go unit test cannot passed in my laptop too even i modified the testcases. it looks like that some setups are missing in my mac. do you have any ideas?
|
Take a look makefile https://github.com/pavolloffay/jaeger-operator/blob/8c81be4b30de53d562074384b482e55c9f10c928/Makefile#L143. the unit test goal uses envtest, probably this is missing in your local setup. |
76be957
to
a8a4d80
Compare
Exactly, Thank you for your infomation, Now all ci unit tests are passed locally.
|
* allow config customized envs on common and language specific node. * only env name start with "OTEL_" is allowed. * deployment env > language spec customzied env > language spec config type> common customized env > common config type * api docs gen and code fmt * use corev1.EnvVar instead of Env * fixing wrong change. * update e2e testcases * fix unit tests
Resolves #657
pod env > language spec customzied env > language spec config> common customized env > common config